Skip to content

gh-148286: Fix undefined behaviour in the ctypes bit field accessors - #154912

Open
matthiasgoergens wants to merge 1 commit into
python:mainfrom
matthiasgoergens:ctypes-bitfield-ub
Open

gh-148286: Fix undefined behaviour in the ctypes bit field accessors#154912
matthiasgoergens wants to merge 1 commit into
python:mainfrom
matthiasgoergens:ctypes-bitfield-ub

Conversation

@matthiasgoergens

@matthiasgoergens matthiasgoergens commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

BIT_MASK, GET_BITFIELD and SET in Modules/_ctypes/cfield.c do their bit manipulation in the bit field's own type. When that type is signed, both shifting a negative value left and shifting a one into the sign bit are undefined behaviour, which is why Tools/ubsan/suppressions.txt carries two file-level entries for this file.

Removing those entries and running test_ctypes under UBSAN_OPTIONS=halt_on_error=1 aborts immediately at cfield.c:644 in i64_set. Collecting rather than halting shows diagnostics at three distinct lines, so the suppression was covering rather more than its comment documents:

cfield.c:644: left shift of 1 by 63 places cannot be represented in type 'int64_t'
cfield.c:644: left shift of negative value -9223372036854775793
cfield.c:640: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
cfield.c:640: left shift of 1 by 31 places cannot be represented in type 'int32_t'
cfield.c:636: left shift of negative value -32768

This does the arithmetic in uint64_t — the widest type the accessors support — and converts back to the field's own type at the end. BIT_MASK becomes LOW_BITS_MASK(), which no longer needs a type argument now that the mask is always computed unsigned; GET_BITFIELD gains the field type so it can still sign-extend signed fields, and is wrapped in do { } while (0).

Since this rewrites the arithmetic of every integer bit field getter and setter, I checked it exhaustively against the old macros rather than relying on review alone. For every fixed-width type from int8_t to uint64_t, every num_bits, every low_bit, ten storage bit patterns and twenty stored values — 1,167,600 cases — the old and new expressions produce identical results, under both gcc and clang. The same sweep under -fsanitize=undefined -fno-sanitize-recover=all is clean, where the old macros produce eleven distinct diagnostics.

With the two Modules/_ctypes/cfield.c entries removed from the suppressions file, test_ctypes passes under UBSAN_OPTIONS=halt_on_error=1.

Provenance

This supersedes #96925, my 2022 attempt at the same bug, which I have now closed. That version worked around the undefined shifts with multiplication, and @mdickinson pushed back on it in review — both because the intent is to move bits rather than to do arithmetic, and because multiplication drags the usual arithmetic conversions in on top of the integer promotions, making the result harder to reason about. He proposed doing the bit manipulation in unsigned types and sign-extending afterwards, and sketched the (v ^ sign_bit) - sign_bit idiom used here.

So the approach in this PR is his rather than mine, four years late. It is also a more complete fix than the old one: #96925 only added an assertion to BIT_MASK and left (type)1 << (NUM_BITS(size) - 1) in place, which is itself undefined for a full-width signed field — that is the left shift of 1 by 63 places diagnostic currently recorded in the suppressions file, so the original patch would not have cleared its own suppression. Thanks, Mark.

…ssors

BIT_MASK, GET_BITFIELD and SET in Modules/_ctypes/cfield.c did their bit
manipulation in the bit field's own type. When that type is signed, both
shifting a negative value left and shifting a one into the sign bit are
undefined behaviour, so UBSan reported, among others:

  cfield.c:644: left shift of 1 by 63 places cannot be represented in 'int64_t'
  cfield.c:640: signed integer overflow: -2147483648 - 1
  cfield.c:636: left shift of negative value -32768

Do the arithmetic in uint64_t -- the widest type the accessors support --
and convert back to the field's own type at the end. BIT_MASK is replaced
by LOW_BITS_MASK(), which no longer needs a type argument now that the
mask is always computed unsigned. GET_BITFIELD takes the field type so it
can still sign-extend signed fields, and is wrapped in do/while(0).

Verified against the old macros with an exhaustive differential test:
for every fixed-width type (int8_t..uint64_t), every num_bits and low_bit
combination, ten storage patterns and twenty stored values -- 1,167,600
cases, no behaviour change, under both gcc and clang.

The two Modules/_ctypes/cfield.c entries in Tools/ubsan/suppressions.txt
are no longer needed. With them removed, test_ctypes passes under
UBSAN_OPTIONS=halt_on_error=1; before this change it aborts at
cfield.c:644 in i64_set.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant